Skip to content

feat(web): add attachment staging lifecycle - #388

Open
testikun wants to merge 6 commits into
openpi-dev:mainfrom
testikun:codex/issue-344-attachment-staging
Open

feat(web): add attachment staging lifecycle#388
testikun wants to merge 6 commits into
openpi-dev:mainfrom
testikun:codex/issue-344-attachment-staging

Conversation

@testikun

@testikun testikun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

The attachment work tracked by #344 has admission metadata validation in review, but no host-owned lifecycle for canonical temporary bytes. Browser filenames must not become filesystem authority, and staged bytes need exact command ownership and deterministic cleanup.

Value

Future composer and multimodal prompt work gets a fail-closed storage primitive with bounded resource use, single-consume semantics, and explicit stale or settled receipts.

Approach

Add a private attachment staging store that:

  • writes payloads under server-generated file identities in a private host directory;
  • binds every batch to the exact canonical workspace, Session id, and command id;
  • serializes stage, consume, discard, and disposal mutations;
  • enforces injected count, per-file, per-command, store-wide byte, and settled-receipt limits;
  • rejects changed files and symlinks before reading;
  • removes staged artifacts on consume, discard, failed integrity checks, and store disposal.

This is a focused backend slice and intentionally does not duplicate #365/#380 metadata admission validation or #384 UI work.

Validation

  • bun run check
  • bun run test: 1245 passed, 0 failed, 1 skipped; Vitest 30 passed
  • focused attachment staging tests: 4 passed
  • git diff --check

Impact

  • User-visible behavior: none until the composer integration lands.
  • Model-visible context/tools: none.
  • Runtime/lifecycle: adds an opt-in host-owned temporary byte lifecycle; no current request path changes.
  • Persisted config/data: none; artifacts are private and temporary.
  • Compatibility/risk: additive module only. Limits are injected so the final upload path can use the authoritative admission policy without a second default.

Related to #344.

@testikun

testikun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

CI context: both Linux Node jobs reach the pre-existing packed standalone Web CLI smoke, install the tarball, and then the server exits before announcing its origin (kill: No such process). Windows and the other checks pass. This matches the independently reproduced fresh-install peer-dependency failure tracked in #328, where npm selects Pi 0.85.0 and @earendil-works/pi-server cannot be resolved. Evidence: #328 (comment)

This PR does not change package metadata, CLI startup, or peer-dependency resolution. Its local check, focused tests, full Node suite, and Vitest suite pass as recorded in the PR body.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要改:暂存没有 TTL,name/mime 也没有自己的上界

web/runtime/attachment-staging.ts

  • 崩溃且没走到 dispose() 会留下 0o700 目录,没有 TTL / 启动回收。
  • validatePayloads 不限制 name / mime,完全指望还没合的 #365/#380

staging 不该自带一份政策,但也不该在准入 PR 落地前把无界元数据写进 store。请先合 #365+#380(或把同样的边界搬进来),并给未 dispose 的根目录加 TTL 或启动清理。

绑定、独占队列、symlink/size fail-closed 这些是对的。

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head re-review of 9ebd185. The previous metadata bounds and missing cleanup findings have been addressed; focused tests pass (5/5). However, the new startup reclamation can delete a different live store and its unconsumed attachments. This is a lifecycle/data-loss blocker, independently reproduced with the real module in a unique temporary parent. No host/UI or deployed impact is claimed; the store is not yet wired to an upload route.

Comment thread web/runtime/attachment-staging.ts
@testikun
testikun force-pushed the codex/issue-344-attachment-staging branch from f8ac1bd to eba69a6 Compare September 6, 2026 06:19
@testikun

testikun commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@tt-a1i 已补齐 TTL、name/mime 256 UTF-8 bytes 限制及回归测试;随后又补上 owner/liveness proof,避免启动回收删除仍存活的 store。

@testikun

testikun commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@tt-a1i 已修复 P1:startup reclaim 现在读取 sibling PID owner marker;owner 存活则保留,旧无 marker 根目录才按 TTL 回收。live-store/orphan 回归通过。

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head review of be9eab7. The staging design has useful binding and cleanup boundaries, but two independently reproduced P2 resource-lifecycle problems remain. Please fix these in this PR and add regression coverage; no author code was modified during review. This module is not yet connected to an upload route, so these are component findings rather than claims about deployed behavior. Related tracking: #441.

Comment thread web/runtime/attachment-staging.ts
Comment thread web/runtime/attachment-staging.ts
@testikun

testikun commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

已推送 51706c6(同步最新 main)。本次处理评审中的两个未解决问题:新增显式 maxStagedBatches 活跃批次数上限,防止零字节批次无限增长;dispose 同时删除 owner marker,并用活跃 store 注册表防止同进程创建新 store 时回收仍在使用的旧 store。新增零字节批次、存活 store TTL、marker 清理回归。专项 attachment-staging 测试 7/7 通过。

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 51706c6.

The host-owned staging primitive is useful and is not wired into current user/model behavior, but two reproducible P1 integrity/lifecycle blockers remain:

  • [P1] Changed bytes can be accepted as canonical. The store records and verifies path/type/size, but not content identity. On the exact head, staging safe, overwriting the payload with same-length evil, then consuming returns consumed with evil. Persist and verify a digest or authoritative opened-file identity, and close the check/read TOCTOU.
  • [P1] discard marks the batch settled before deletion succeeds. If rm fails, the first call rejects, but a retry reports settled/discarded while the bytes remain. Only settle after cleanup succeeds, or persist an explicit failed/retryable cleanup state.

[P2] Workspace ownership is also raw absolute-string equality rather than a proven canonical path, so aliases can become distinct owners.

Focused tests and required CI are green, but they do not cover these exact failure cases. The P1s block merging this as the canonical attachment-byte authority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants